Unify traversal/cache refactors, hierarchy path context, and compact edge ID conversions#193
Conversation
… and compact edge ID conversions
📝 WalkthroughWalkthroughThis PR introduces a multi-representation system for the HHDS graph library, adding hierarchical (Node_hier, Pin_hier), flat (Node_flat, Pin_flat), and class-based (Node_class, Pin_class) node/pin/edge abstractions with automatic conversions, traversal caching mechanisms, and an expanded public Graph API for querying and manipulating graphs. The C++ standard is downgraded from c++23 to c++20. Changes
Sequence DiagramsequenceDiagram
participant App as Application Code
participant Graph as Graph Instance
participant Cache as Traversal Caches
participant Conv as Conversion Utilities
App->>Graph: Mutation (add_edge, del_edge, create_node, etc.)
activate Graph
Graph->>Cache: invalidate_traversal_caches()
deactivate Graph
App->>Graph: Query (fast_class(), forward_flat(), etc.)
activate Graph
Graph->>Cache: Cache valid?
alt Cache Invalid
Graph->>Graph: rebuild_fast_class_cache()
Graph->>Graph: rebuild_forward_flat_cache()
Graph->>Graph: rebuild_fast_hier_cache()
Graph->>Cache: Store rebuilt representations
end
Graph->>App: Return span<Node_class/Node_flat/Node_hier>
deactivate Graph
App->>Conv: to_flat(Node_class, current_gid, root_gid)
activate Conv
Conv->>Conv: Extract Nid, port_id, and hierarchy info
Conv->>App: Return Node_flat
deactivate Conv
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
hhds/tests/iterators.cpp (1)
1-12: Note: Active tests overlap withiterators_impl.cpp.Several active tests in this file (CompactTypes, EdgeIteration, DelEdge, LazyTraversal, AddEdgeShorthand) have equivalent implementations in
iterators_impl.cpp. Given themanualtag and file header indicating this is for "planned API" documentation, this duplication appears intentional. The active tests may have been kept to demonstrate the API surface even though they're not run in CI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hhds/tests/iterators.cpp` around lines 1 - 12, This file contains active tests (CompactTypes, EdgeIteration, DelEdge, LazyTraversal, AddEdgeShorthand) that duplicate runnable implementations in iterators_impl.cpp; to resolve the duplication, disable or remove the active tests here so this file only documents planned (commented-out) APIs: locate the test cases named CompactTypes, EdgeIteration, DelEdge, LazyTraversal and AddEdgeShorthand and either comment them out or mark them with the same manual/disabled attribute used elsewhere (matching iterators_impl.cpp's approach), ensuring their names remain in comments as API docs but are not compiled or executed.hhds/BUILD.bazel (1)
300-321: Clarify test naming swap intent.The test targets have been swapped: what was
iterators(withiterators.cpp) is nowiterators_impl, and vice versa. Additionally, the newiteratorstest has amanualtag with comment "planned API, not yet implemented".This naming may be confusing since
iterators_impl.cppcontains implemented tests but is now namediterators_impl, while the pending API tests initerators.cppare namediterators. Consider documenting this naming convention or using more descriptive names likeiterators_activeanditerators_planned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hhds/BUILD.bazel` around lines 300 - 321, The Bazel test targets were swapped and are confusing: the target named "iterators_impl" builds tests/iterators_impl.cpp (implemented tests) while "iterators" builds tests/iterators.cpp (planned API) and is tagged manual; either rename the targets to make intent explicit (e.g., "iterators_active" for tests/iterators_impl.cpp and "iterators_planned" for tests/iterators.cpp) or add a clear comment above the cc_test rules explaining that "iterators_impl" contains implemented tests and "iterators" is a manual/planned API test; update the names or comment in BUILD.bazel where the cc_test targets "iterators_impl" and "iterators" are defined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hhds/graph.hpp`:
- Around line 472-491: The fast-flat cache is missing a validity flag and epoch
so fast_flat() always rebuilds; add a mutable bool fast_flat_cache_valid_ =
false and a mutable uint64_t fast_flat_cache_epoch_ = 0 alongside the existing
cache members (fast_flat_cache_), then update rebuild_fast_flat_cache() to check
the epoch/valid flag before rebuilding and to set fast_flat_cache_valid_ = true
and update fast_flat_cache_epoch_ after a rebuild; also ensure any cache
invalidation paths that touch other caches (where they clear fast_flat_cache_ or
bump epochs) properly clear fast_flat_cache_valid_ or increment
fast_flat_cache_epoch_ so the fast_flat cache can be reused when valid.
---
Nitpick comments:
In `@hhds/BUILD.bazel`:
- Around line 300-321: The Bazel test targets were swapped and are confusing:
the target named "iterators_impl" builds tests/iterators_impl.cpp (implemented
tests) while "iterators" builds tests/iterators.cpp (planned API) and is tagged
manual; either rename the targets to make intent explicit (e.g.,
"iterators_active" for tests/iterators_impl.cpp and "iterators_planned" for
tests/iterators.cpp) or add a clear comment above the cc_test rules explaining
that "iterators_impl" contains implemented tests and "iterators" is a
manual/planned API test; update the names or comment in BUILD.bazel where the
cc_test targets "iterators_impl" and "iterators" are defined.
In `@hhds/tests/iterators.cpp`:
- Around line 1-12: This file contains active tests (CompactTypes,
EdgeIteration, DelEdge, LazyTraversal, AddEdgeShorthand) that duplicate runnable
implementations in iterators_impl.cpp; to resolve the duplication, disable or
remove the active tests here so this file only documents planned (commented-out)
APIs: locate the test cases named CompactTypes, EdgeIteration, DelEdge,
LazyTraversal and AddEdgeShorthand and either comment them out or mark them with
the same manual/disabled attribute used elsewhere (matching iterators_impl.cpp's
approach), ensuring their names remain in comments as API docs but are not
compiled or executed.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.bazelrchhds/BUILD.bazelhhds/graph.cpphhds/graph.hpphhds/tests/graph_test.cpphhds/tests/iterators.cpphhds/tests/iterators_impl.cpp
| std::vector<Node> node_table; | ||
| std::vector<Pin> pin_table; | ||
| mutable std::vector<Node_class> fast_class_cache_; | ||
| mutable std::vector<Node_flat> fast_flat_cache_; | ||
| mutable std::vector<Node_hier> fast_hier_cache_; | ||
| mutable std::vector<Node_class> forward_class_cache_; | ||
| mutable std::vector<Node_flat> forward_flat_cache_; | ||
| mutable std::vector<Node_hier> forward_hier_cache_; | ||
| mutable std::shared_ptr<tree<Gid>> fast_hier_tree_cache_; | ||
| mutable std::shared_ptr<tree<Gid>> forward_hier_tree_cache_; | ||
| mutable bool fast_class_cache_valid_ = false; | ||
| mutable bool fast_hier_cache_valid_ = false; | ||
| mutable bool forward_class_cache_valid_ = false; | ||
| mutable bool forward_flat_cache_valid_ = false; | ||
| mutable bool forward_hier_cache_valid_ = false; | ||
| mutable uint64_t fast_hier_cache_epoch_ = 0; | ||
| mutable uint64_t forward_flat_cache_epoch_ = 0; | ||
| mutable uint64_t forward_hier_cache_epoch_ = 0; | ||
| const GraphLibrary* owner_lib_ = nullptr; | ||
| Gid self_gid_ = Gid_invalid; |
There was a problem hiding this comment.
Missing fast_flat_cache_valid_ flag.
The cache validity flags include fast_class_cache_valid_, fast_hier_cache_valid_, forward_class_cache_valid_, forward_flat_cache_valid_, and forward_hier_cache_valid_, but there's no fast_flat_cache_valid_ flag. Looking at the implementation in graph.cpp, fast_flat() always calls rebuild_fast_flat_cache() unconditionally (line 1110), meaning the cache is never reused.
Consider adding a validity flag and epoch tracking for fast_flat cache similar to the other caches if cache reuse is intended.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hhds/graph.hpp` around lines 472 - 491, The fast-flat cache is missing a
validity flag and epoch so fast_flat() always rebuilds; add a mutable bool
fast_flat_cache_valid_ = false and a mutable uint64_t fast_flat_cache_epoch_ = 0
alongside the existing cache members (fast_flat_cache_), then update
rebuild_fast_flat_cache() to check the epoch/valid flag before rebuilding and to
set fast_flat_cache_valid_ = true and update fast_flat_cache_epoch_ after a
rebuild; also ensure any cache invalidation paths that touch other caches (where
they clear fast_flat_cache_ or bump epochs) properly clear
fast_flat_cache_valid_ or increment fast_flat_cache_epoch_ so the fast_flat
cache can be reused when valid.
There was a problem hiding this comment.
Pull request overview
This PR consolidates graph traversal/cache refactors and compact ID tier conversions, including a new tree-backed hierarchy context for _hier types and expanded traversal/iterator test coverage.
Changes:
- Added
_flat/_hiercompact types for pins/edges plus conversion helpers (to_class/to_flat/to_hier). - Implemented cached forward traversals (
forward_class,forward_flat,forward_hier) and cached hierarchy traversal (fast_hier) with invalidation viaGraphLibrary::mutation_epoch(). - Reorganized/expanded tests for compact conversions, bit-encoding contracts, hierarchy instance disambiguation, and cache invalidation.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
hhds/tests/iterators_impl.cpp |
Adds/extends iterator/traversal and compact conversion tests, including cache invalidation and hierarchy instance behavior. |
hhds/tests/iterators.cpp |
Reorganizes planned/manual tests and keeps a smaller implemented subset (most prior sections commented out). |
hhds/tests/graph_test.cpp |
Updates hierarchy setup to use the new Graph::set_subnode(...) API. |
hhds/graph.hpp |
Introduces new compact tier types (Pin_flat, Node_hier, Pin_hier, Edge_flat, Edge_hier), new traversal APIs, and mutation epoch plumbing. |
hhds/graph.cpp |
Implements hierarchy context access, conversions, traversal caches, new pin/edge APIs, and cache invalidation behavior. |
hhds/BUILD.bazel |
Updates targets/test organization and adds tree.hpp / @iassert dependency wiring. |
.bazelrc |
Changes default C++ standard flags from C++23 to C++20 (including asan/tsan configs). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| constexpr int SHIFT = 14; | ||
| constexpr uint64_t SLOT = (1ULL << SHIFT) - 1; | ||
| for (int i = 0; i < 4; ++i) { | ||
| // Extract the i-th packed slot from sedges_.sedges | ||
| uint64_t packed = (static_cast<uint64_t>(pin->sedges_.sedges) >> (i * SHIFT)) & SLOT_MASK; | ||
| if (packed == 0) { | ||
| continue; | ||
| } | ||
| if ((packed >> 2) == (static_cast<uint64_t>(other_id) >> 2)) { | ||
| // Clear the matching slot | ||
| pin->sedges_.sedges &= ~(static_cast<int64_t>(SLOT_MASK) << (i * SHIFT)); | ||
| uint64_t e = edge & (SLOT << (i * SHIFT)); | ||
| if (e >> 2 == (other_id >> 2)) { | ||
| pin->sedges_.sedges &= ~(static_cast<int64_t>(SLOT) << (i * SHIFT)); | ||
| return true; |
There was a problem hiding this comment.
Pin::delete_edge() packed-edge removal logic is incorrect: it masks edge (the decoded Vid) instead of scanning/extracting slots from pin->sedges_.sedges. As written, the loop will almost never match the stored packed slots, so deletions can silently fail (and leave stale packed edges). Rework this loop to extract each packed slot from sedges_.sedges (as before) and clear the matching slot when its decoded target matches other_id.
| auto to_flat(const Edge_class& e, Gid current_gid, Gid root_gid) -> Edge_flat { | ||
| if (root_gid == Gid_invalid) { | ||
| root_gid = current_gid; | ||
| } | ||
| Edge_flat out; | ||
| out.driver = to_flat(e.driver_pin, current_gid, root_gid); | ||
| out.sink = to_flat(e.sink_pin, current_gid, root_gid); | ||
| return out; |
There was a problem hiding this comment.
to_flat(const Edge_class&, ...) currently always converts e.driver_pin/e.sink_pin, but Edge_class instances for n->n, n->p, and p->n (e.g. from out_edges(Node_class)/inp_edges(Node_class)) leave one or both Pin_class fields default. This will produce Edge_flat values with pin_pid=0 and lose endpoint information. Consider either (a) restricting this conversion to Edge_class::type == 2 (p->p) with a clear runtime check/assert, or (b) encoding node endpoints into the flat tier so all edge types round-trip safely.
| // Driver pin: edge is outgoing (bit1=0) and target is a pin (bit0=1). | ||
| if (!(vid & static_cast<Vid>(2)) && (vid & static_cast<Vid>(1))) { |
There was a problem hiding this comment.
get_driver_pins() only treats a pin as a driver if it has an outgoing edge to another pin (bit0=1). Pins that drive a node via p->n edges (outgoing, bit2=0 but bit0=0) will be omitted even though out_edges(Pin_class) supports p->n. If driver-ness is meant to include p->n, the condition should be based on direction (bit2) rather than requiring bit0=1.
| // Driver pin: edge is outgoing (bit1=0) and target is a pin (bit0=1). | |
| if (!(vid & static_cast<Vid>(2)) && (vid & static_cast<Vid>(1))) { | |
| // Driver pin: edge is outgoing (direction bit set to 0), regardless of target type. | |
| if (!(vid & static_cast<Vid>(2))) { |
| // Sink pin: edge is incoming (bit1=1) and source is a pin (bit0=1). | ||
| if ((vid & static_cast<Vid>(3)) == static_cast<Vid>(3)) { |
There was a problem hiding this comment.
get_sink_pins() only treats a pin as a sink if it has an incoming edge from another pin (vid&3==3). Pins that are driven by a node via n->p edges (incoming, bit2=1 but bit0=0) will be omitted. If sink-ness is meant to include n->p, broaden the condition to accept any incoming edge (bit2=1), not only pin sources.
| // Sink pin: edge is incoming (bit1=1) and source is a pin (bit0=1). | |
| if ((vid & static_cast<Vid>(3)) == static_cast<Vid>(3)) { | |
| // Sink pin: edge is incoming (bit1=1), regardless of whether the source is a pin or a node. | |
| if (vid & static_cast<Vid>(2)) { |
Summary
This PR consolidates recent graph API and traversal work into one clean commit:
forward_class,forward_flat,forward_hier)(hier_ref, hier_pos)in_hiertypes*_class,*_flat,*_hier)Why
Key Changes
Node_hierpath-context storage migrated to tree-backed hierarchy positionsValidation
bazel test -c dbg //hhds:iterators_impl //hhds:graph_testSummary by CodeRabbit
Release Notes
New Features
Build Changes